Get SpaceDock co-authors from POST form lists #297
Conversation
HebaruSan
added
Bug
There was a problem hiding this comment.
Do we have a sample spacedock payload for this? I wouldn't mind constructing a test case. I did some looking and it's noteable the list ends up empty if a key is missing
[{'username':x[0], 'user_github': x[1]} for x in zip(raw.getlist('username'), raw.getlist('user_github'))]
[]vs
[{'username':x[0]} for x in zip(raw.getlist('username'))]
[{'username': 'modauthor1'}, {'username': 'another_user'}]What's the complexity involved in using JSON instead? I must admit that I was tripped up by it being a forms based endpoint.
| in zip(raw.getlist('username'), | ||
| raw.getlist('user_github'), | ||
| raw.getlist('user_forum_id'), | ||
| raw.getlist('user_forum_username'), | ||
| raw.getlist('email'))]}) |
There was a problem hiding this comment.
Are all these keys going to be present if there are multiple authors?
There was a problem hiding this comment.
That's the plan, yeah. They're already sent unconditionally, regardless of if there is one author or twelve, and KSP-SpaceDock/SpaceDock#490 is just making them contain lists:
https://github.com/KSP-SpaceDock/SpaceDock/pull/490/files
There was a problem hiding this comment.
I've crafted a test for this, intended destination tests.webhooks.TestWebhookSpaceDockAdd
@mock.patch('netkan.webhooks.config.WebhooksConfig.client')
def test_multi_authors_forms(self, queued: MagicMock):
data = self.mock_netkan_hook()
data.update({
'username': ['author1', 'author2'],
'user_github': ['author1_gh']
})
response = self.client.post('/sd/add/ksp', data=data)
self.assertEqual(response.status_code, 204)
call = queued.method_calls.pop().call_list().pop()
body = json.loads(call[2].get('Entries')[0].get('MessageBody'))
self.assertListEqual(
body.get('all_authors'),
[{'username': 'author1', 'user_github': 'author1_gh',
'email': 'modauthor1@gmail.com'}, {'username': 'author2'}]
)Which fails:
AssertionError: Lists differ: [] != [{'username': 'author1', 'user_github': 'author1_gh'}, {'username': 'author2'}]
Second list contains 2 additional elements.
First extra element 0:
{'username': 'author1', 'user_github': 'author1_gh'}
- []
+ [{'user_github': 'author1_gh', 'username': 'author1'}, {'username': 'author2'}]
The problem is the zip, it'll return a tuple with a length equal to the item with the least items. Which will be fine if the lists come through in equal lengths, regardless of if an author is missing a github or something.
If the payload comes through more like this, the lists will all be equal
('user', 'author1'), ('user', 'author2'),('user_github', 'author3')
('user_github', 'author1_gh'), ('user_github', ''),('user_github', 'author3_gh')
('email', 'author1@email'), ('email', ''),('email': '')
('user_forum_id', ''), ('user_forum_id', ''),(user_forum_id': '')
('user_forum_username', ''), ('user_forum_username', ''),('user_forum_username': '')
|
The main thing that makes me hesitate to switch to JSON is that I don't know how to do that transition without some downtime of the link (and having to rewrite stuff). If we change SpaceDock first, then Infra would start receiving JSON and not know what to do about it; if we change Infra first, then it would break until SpaceDock started sending JSON. If we stick with POST forms, we can safely change SpaceDock first, since This stuff is documented to be a POST form, I just failed to read the comment: NetKAN-Infra/netkan/netkan/webhooks/spacedock_add.py Lines 19 to 35 in 38aa44b |
We'd inspect the content type. Requests will do the RightThing ™️ and set it to
Oh I read and understood the structure. But I'm not super experienced with forms + arrays. Without an expected payload it's kinda tricky to be certain I'm testing the same thing. It's doesn't cause a critical failure if there were a problem, just all authors would end up empty. |
|
We have a prior example of SpaceDock↔CKAN communication with POST form lists, but in the other direction, in the batched download counts (see #228 and KSP-SpaceDock/SpaceDock#401). CKAN sends a list and SpaceDock receives it with |
|
Oh I think the approach is ok, just the structure is a bit janky to work with IMO. And if we're making changes, JSON would be nicer. I'll chip away at a test case and see if I'm misunderstanding something. |
Problem
See #293 and #294; #287 doesn't work. Instead of the
shared_authorsproperty containing the co-authors, we just receiveusername, so attempting to add the co-authors throws an exception.Cause
You can't send a list of dicts in a POST form. I thought it was JSON.
Changes
Someday soon, SpaceDock will start sending multiple values for the user fields, one per author (see KSP-SpaceDock/SpaceDock#490). This is supported in POST forms. Once we have that, this pull request will read those lists to get the co-authors.
Note that KSP-SpaceDock/SpaceDock#490 must be deployed to production before this is merged! Currently there is no specific plan for the next upgrade, but SpaceDock updates have been somewhat frequent lately, so it shouldn't take too long.